Add transfer type payload conversion hooks - #795
Conversation
2c28d33 to
d1228be
Compare
tconley1428
changed the title
932f77e to
ceb5be8
Compare
| } | ||
| } | ||
|
|
||
| options.DataConverter = TemporalTransferTypePayloadConverter.Wrap(options.DataConverter); |
There was a problem hiding this comment.
I think we should clone the options and replace the data converter on the clone because if the caller looks at the data converter after constructing the client, they will see a different value on their options instance. We should probably make the clone before plugins operate on the options.
| /// </summary> | ||
| /// <param name="connection">Connection for this client.</param> | ||
| /// <param name="options">Options for this client.</param> | ||
| public TemporalClient(ITemporalConnection connection, TemporalClientOptions options) |
There was a problem hiding this comment.
I think this is looking better. One thing I'm thinking through is if the customer constructs another client based on the options that the first client has AND a plugin mutates the PayloadConverter, we'll have nested special internal payload converter within the chain and one as the very outermost instance. For example:
var client1 = new TemporalClient(connection, new TemporalClientOptions
{
DataConverter = new DataConverter(inner, new DefaultFailureConverter()),
});
var client2 = new TemporalClient(connection, new TemporalClientOptions
{
DataConverter = client1.Options.DataConverter,
Plugins = new[] { plugin },
});If plugin wraps the PayloadConverter during ConfigureClient, we end up with a PayloadConverter on client2 that looks like W(P(W(inner))) where W is the TemporalTransferTypePayloadConverter and P is the plugin payload converter. This is probably benign and not likely to happen all that often.
There was a problem hiding this comment.
If we want to completely fix this, might need to have an internal ResolvedDataConverter property on the options that is used internally throughout the library.
| public NoJsonProtoPayloadConverter() | ||
| : base( | ||
| ((DefaultPayloadConverter)DataConverter.Default.PayloadConverter). | ||
| new DefaultPayloadConverter(). |
There was a problem hiding this comment.
This can probably be reverted.
| // Context-aware data converter and client | ||
| var dataConverter = new DataConverter( | ||
| PayloadConverter: new DefaultPayloadConverter( | ||
| ((DefaultPayloadConverter)Client.Options.DataConverter.PayloadConverter).EncodingConverters.Select(e => |
There was a problem hiding this comment.
This demonstrates part of the problem I wrote up in https://github.com/temporalio/sdk-dotnet/pull/795/changes#r3677825054. If we keep this behavior, maybe just use DataConverter.Default.PayloadConverter here.
2 participants
Adds experimental SDK payload-converter support for values and target types marked with
TemporalTransferTypeAttribute.The attribute points at an
ITemporalTransferTypeConverterimplementation. The converter supplies theTransferTypehanded to the configured payload converter, converts user values into that representation for encoding, and reconstructs user values from it for decoding. General transfer type hooks intentionally do not receive a payload converter.This keeps normal SDK payload-converter behavior in one place, including codec handling and serialization-context propagation.
This change:
DataConverter, includingwith { PayloadConverter = ... }TemporalTransferTypeAttributeandITemporalTransferTypeConverterAPIs